Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrades jest to v27 #1301

Merged
merged 1 commit into from
Mar 3, 2022
Merged

Upgrades jest to v27 #1301

merged 1 commit into from
Mar 3, 2022

Conversation

tmarkley
Copy link
Contributor

@tmarkley tmarkley commented Mar 1, 2022 •
edited
Loading

Description

  • Upgrades jest from v26.4.2 to v27.5.1.
    • CHANGELOG
    • v27 introduces breaking changes that have to be addressed.
  • Upgrades testing-related dependencies, removes unused dependency babel-plugin-istanbul, and replaces jest-environment-jsdom-thirteen with the built-in jest-environment-jsdom.
  • Skips a few flaky tests that require additional investigation.

Issues Resolved

Prerequisite for addressing #1231

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@tmarkley tmarkley added test:unit test:integration dependencies Pull requests that update a dependency file v2.0.0 labels Mar 1, 2022
@tmarkley tmarkley requested a review from a team as a code owner March 1, 2022 04:30
@tmarkley tmarkley force-pushed the test_workflow_performance branch from 45ec3a3 to 4375353 Compare March 1, 2022 16:24
@tmarkley tmarkley mentioned this pull request Mar 1, 2022
Closed
7 tasks
@tmarkley
Copy link
Contributor Author

tmarkley commented Mar 1, 2022

Note: the WhiteSource Security Check is incorrect; a version of ansi-regex was removed, so that CVE was not introduced in this commit (it existed before: #1084).

ananzh
ananzh previously approved these changes Mar 2, 2022
View reviewed changes
@tmarkley tmarkley force-pushed the test_workflow_performance branch from 4375353 to 8da108a Compare March 2, 2022 00:53
@tmarkley tmarkley requested review from ananzh and kavilla March 2, 2022 00:54
ananzh
ananzh previously approved these changes Mar 2, 2022
View reviewed changes
kavilla
kavilla reviewed Mar 2, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we, if you have already, check on 1.x the runInBand doesn't cause any issue? I believe it did but perhaps it's worth 1 more pass. If so would it be worth to have 1 commit or PR updating the github workflow another to upgrading jest. Since this can't be backported.

.eslintrc.js Show resolved Hide resolved
.github/workflows/pr_check_workflow.yml Outdated Show resolved Hide resolved
@tmarkley
Copy link
Contributor Author

tmarkley commented Mar 2, 2022 •
edited
Loading

Can we, if you have already, check on 1.x the runInBand doesn't cause any issue? I believe it did but perhaps it's worth 1 more pass. If so would it be worth to have 1 commit or PR updating the github workflow another to upgrading jest. Since this can't be backported.

Yeah I can split this up into two PRs! And yes I can check 1.x and see how things work.

@tmarkley tmarkley force-pushed the test_workflow_performance branch 2 times, most recently from e6fb6af to e670833 Compare March 2, 2022 16:54
@tmarkley tmarkley changed the title Upgrades jest to v27, simplifies GitHub workflow Upgrades jest to v27 Mar 2, 2022
@tmarkley tmarkley force-pushed the test_workflow_performance branch 2 times, most recently from ec39ceb to 2ca8c3c Compare March 2, 2022 17:16
@tmarkley tmarkley requested review from kavilla, ananzh and ashwin-pc March 2, 2022 17:55
@tmarkley tmarkley force-pushed the test_workflow_performance branch from 2ca8c3c to 2645102 Compare March 2, 2022 19:29
kavilla
kavilla reviewed Mar 2, 2022
View reviewed changes
src/core/server/http/integration_tests/request.test.ts
@@ -180,24 +187,25 @@ describe('OpenSearchDashboardsRequest', () => {

describe('events', () => {
describe('aborted$', () => {
it('emits once and completes when request aborted', async (done) => {
// Flaky test
it.skip('emits once and completes when request aborted', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these flaky only in the CI? I was able to get the passing locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you pulled the PR down it consistently passed locally for you? Sometimes it would pass, sometimes it would fail for me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate on the failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test would timeout after 30 seconds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I believe we have to make an update still: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1301/files#r818949420

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still couldn't get this one to pass. This test doesn't use fake timers.

Copy link
Contributor Author

@tmarkley tmarkley Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be related to the supertest functions used, but I haven't narrowed it down.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be related to the supertest functions used, but I haven't narrowed it down.

If you aren't able to for this PR could we create a follow up issue to look into it? I don't want to block this PR any longer because it prevents an improvement that brings in more value than this test can provide but I don't want to forget about this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment for this specifically in the issue we already have open for these types of cases: #5

@tmarkley tmarkley requested review from kavilla and AMoo-Miki March 2, 2022 23:13
ananzh
ananzh previously approved these changes Mar 3, 2022
View reviewed changes
Upgrades jest to v27
eeb5144 
* Upgrades `jest` from v26.4.2 to v27.5.1.
  * [CHANGELOG](https://github.com/facebook/jest/blob/v27.5.1/CHANGELOG.md)
  * v27 introduces breaking changes that have to be addressed.
* Upgrades testing-related dependencies, removes unused dependency
`babel-plugin-istanbul`, and replaces `jest-environment-jsdom-thirteen`
with the built-in `jest-environment-jsdom`.

This is a prerequisite to resolving opensearch-project#1231

Signed-off-by: Tommy Markley <[email protected]>
@tmarkley tmarkley force-pushed the test_workflow_performance branch from 2645102 to eeb5144 Compare March 3, 2022 20:56
@tmarkley tmarkley requested review from AMoo-Miki, ananzh and kavilla March 3, 2022 20:59
kavilla
kavilla approved these changes Mar 3, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks! LGTM. Just the comment about creating a follow up issue. And probably something will need to be included as a heads up here: #1206?

@tmarkley tmarkley mentioned this pull request Mar 3, 2022
Closed
4 tasks
@tmarkley tmarkley merged commit 082f44a into opensearch-project:main Mar 3, 2022
@tmarkley tmarkley deleted the test_workflow_performance branch March 3, 2022 22:21
@tmarkley tmarkley mentioned this pull request Mar 14, 2022
Merged
7 tasks
@tmarkley tmarkley added the cve Security vulnerabilities detected by Dependabot or Mend label Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ananzh ananzh ananzh approved these changes

@kavilla kavilla kavilla approved these changes

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

Assignees
No one assigned
Labels
cve Security vulnerabilities detected by Dependabot or Mend dependencies Pull requests that update a dependency file test:integration test:unit v2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmarkley @AMoo-Miki @kavilla @ananzh